Added fixes for hybrid level coordinates of CESM2 models by schlunma · Pull Request #882 · ESMValGroup/ESMValCore

schlunma · 2020-11-25T15:49:35Z

This PR adds fixes for cl, cli and clw for the CESM2 models.

Tasks

Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
This pull request has a descriptive title that can be used in a changelog
Add unit tests
Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
If writing a new/modified preprocessor function, please update the documentation
Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
Please use yamllint to check that your YAML files do not contain mistakes
If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.

Closes ESMValGroup/ESMValTool#1903.

axel-lauer

Looks fine to me. This is a good approach to be able to work with the affected model datasets as we are not able to provide "fixes" for this issue for the affected models. Without this fix, some variables from these models could not be processed.

zklaus

I am still in the process of evaluation. My findings so far: The data from the CESM models are all broken. The lev coordinate has an invalid standard name that does not refer to a parametric coordinate, the lev bounds use a different coordinate system from the lev coordinate itself.

As such, those models need to be fixed independently from any action suggested in this PR.

That leaves us with only the CAS model. I have not finished looking into that.

As such, at the moment it is unclear if this PR will address an actual problem.

ruthlorenz · 2020-12-07T10:33:44Z

If this is an issue with the data which could (potentially) be solved by the modelling group it should be reported to them. We have done this in the past and the modelling groups are generally happy to be informed and try to fix things if they can. They then also add the information here https://errata.es-doc.org/static/index.html so other people have a chance of knowing these issues.

schlunma · 2021-02-26T17:51:25Z

I think I found a solution to this problem:

As stated in Appendix D of the CF conventions, "the hybrid sigma-pressure coordinate for level k is defined as a(k)+b(k) or ap(k)/p0+b(k), as appropriate".

I checked this, and it is true for most CMIP6 models (the ones differing are the CESM2 models, the CAS model and 1 other model where there are numerical inconsistencies). This should allow us to calculate the correct values for the atmosphere_hybrid_sigma_pressure_coordinate for CESM2 and CAS in a fix, which will solve this problem. I will open a PR on Monday.

However, I really think that in some very specific cases it might not be useful or user-friendly to check all nitty gritty aspects of the CMOR table when there is a scientific justification. In this specific case, the height information was always given (by the formula which does NOT depend on atmosphere_hybrid_sigma_pressure_coordinate) and unique, so checking the values of the atmosphere_hybrid_sigma_pressure_coordinate is really not useful. It probably took me now a day now to get an idea how to "fix" a dataset that is not wrong from a scientific point of view, which is kind of annoying.

sloosvel · 2021-03-01T07:54:31Z

As stated in Appendix D of the CF conventions, "the hybrid sigma-pressure coordinate for level k is defined as a(k)+b(k) or ap(k)/p0+b(k), as appropriate".

That was pointed out in #840. Back in the day I tried summing a+b and I got coordinate values that match those of other models, so if that works for you, we can add this type of fix instead of skipping them. Copying the values from a model with the right coordinate as a fix would not be that bad either.

schlunma · 2021-03-01T15:01:17Z

Will re-open soon, this was automatically closed by force-pushing the current master to it (so I can have a fresh start on this branch).

schlunma · 2021-03-01T16:32:36Z

I added the fix for the CESM2 models according to the link that @sloosvel (I didn't see that back then, sorry). Unfortunately the CAS model is not available on mistral anymore, so I cannot test this.

@sloosvel can you do a technical review, please?

@axel-lauer can you do a scientific review, please? I used this recipe for testing:

recipe_test_cl_cli_clw.yml.txt

sloosvel

I could only run the CESM2 dataset, as the others are missing on jasmin, but the fix looks good to me.

axel-lauer

I have tried this fix and it produces results that look good when using no preprocessor. When using e.g. the following preprocessor to convert the data to altitude levels (same happens for pressure levels), only the lowermost level contains valid values while all other levels contain only missing values:

    extract_levels:
      levels: {cmor_table: CMIP6, coordinate: alt40}
      coordinate: altitude
      scheme: linear

This behavior differs from the results obtained using the current master branch (with --check_level=ignore to get the tool to process cl, cli or clw from CESM2). With the master branch, all altitude / pressure levels contain valid values.

I currently do not understand why this happens, but I think this might need some more investigation before this fix can be merged.

schlunma · 2021-03-04T08:46:43Z

That is really strange....having a look at this right now!

axel-lauer

After talking to @schlunma I redid the tests with a clean version of this branch (fix_hybrid_lev_coords). This solved the problem, the results now look good and as expected. It seems the problem I reported yesterday was caused by a somewhat screwed up version of my local repository copy. Sorry about that. This fix can be merged now!

schlunma · 2021-03-04T13:56:37Z

@zklaus can you either approve the changes or dismiss you review? Merging is blocked because you requested changes on the original version of this PR which is completely outdated now.

axel-lauer · 2021-03-11T08:11:44Z

We have positive reviews and we need this PR now for further work. Since the PR changed in a way that the changes requested by @zklaus for an earlier version do not apply any more, I think merging this now is fine.

schlunma added the cmor Related to the CMOR standard label Nov 25, 2020

schlunma requested a review from jvegreg as a code owner November 25, 2020 15:49

schlunma self-assigned this Nov 25, 2020

bouweandela requested review from sloosvel and zklaus November 25, 2020 16:32

This comment has been minimized.

Sign in to view

schlunma mentioned this pull request Nov 26, 2020

Generic level checks lead to inconsistent ESMValTool runs #883

Closed

This comment has been minimized.

Sign in to view

schlunma requested a review from axel-lauer November 30, 2020 15:28

axel-lauer approved these changes Dec 2, 2020

View reviewed changes

zklaus requested changes Dec 2, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

bouweandela mentioned this pull request Dec 7, 2020

Monthly ESMValtool meeting December ESMValGroup/ESMValTool#1900

Closed

axel-lauer mentioned this pull request Feb 26, 2021

Monthly ESMValTool meeting March ESMValGroup/ESMValTool#2009

Closed

schlunma closed this Mar 1, 2021

schlunma force-pushed the fix_hybrid_lev_coords branch from f0a061d to 2966f54 Compare March 1, 2021 15:00

Fixed hybrid sigma pressure coordinate of CESM2 models

aed1f33

schlunma reopened this Mar 1, 2021

schlunma removed the request for review from jvegreg March 1, 2021 16:29

schlunma requested review from axel-lauer, sloosvel and zklaus and removed request for sloosvel and zklaus March 1, 2021 16:29

schlunma added fix for dataset Related to dataset-specific fix files and removed cmor Related to the CMOR standard labels Mar 1, 2021

schlunma changed the title ~~Removed coordinate data checks for hybrid level coordinates~~ Added fixes for hybrid level coordinates of CESM2 models Mar 1, 2021

sloosvel approved these changes Mar 2, 2021

View reviewed changes

axel-lauer requested changes Mar 3, 2021

View reviewed changes

axel-lauer approved these changes Mar 4, 2021

View reviewed changes

axel-lauer merged commit 5abc853 into master Mar 11, 2021

axel-lauer deleted the fix_hybrid_lev_coords branch March 11, 2021 08:11

bouweandela mentioned this pull request Oct 17, 2024

Dataset problem: Hybrid sigma pressure coordinate points in CESM models #840

Closed

Uh oh!

Conversation

schlunma commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

axel-lauer left a comment

Choose a reason for hiding this comment

Uh oh!

zklaus left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ruthlorenz commented Dec 7, 2020

Uh oh!

schlunma commented Feb 26, 2021

Uh oh!

sloosvel commented Mar 1, 2021

Uh oh!

schlunma commented Mar 1, 2021

Uh oh!

schlunma commented Mar 1, 2021

Uh oh!

sloosvel left a comment

Choose a reason for hiding this comment

Uh oh!

axel-lauer left a comment

Choose a reason for hiding this comment

Uh oh!

schlunma commented Mar 4, 2021

Uh oh!

axel-lauer left a comment

Choose a reason for hiding this comment

Uh oh!

schlunma commented Mar 4, 2021

Uh oh!

axel-lauer commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

schlunma commented Nov 25, 2020 •

edited

Loading